Skip to content

ARROW-11695: [C++][FlightRPC] fix option to disable TLS verification#9569

Closed
lidavidm wants to merge 1 commit into
apache:masterfrom
lidavidm:arrow-11695-detection
Closed

ARROW-11695: [C++][FlightRPC] fix option to disable TLS verification#9569
lidavidm wants to merge 1 commit into
apache:masterfrom
lidavidm:arrow-11695-detection

Conversation

@lidavidm

@lidavidm lidavidm commented Feb 24, 2021

Copy link
Copy Markdown
Member

gRPC 1.34 and 1.36 both change up the API, so we have to detect both of those versions. The CMake config and C++ code was also refactored a bit so that there's less to copy-paste for each gRPC version change.

@lidavidm

lidavidm commented Feb 24, 2021

Copy link
Copy Markdown
Member Author

CC @xhochy; I've added a CMake flag to fail the build if a supported gRPC version isn't detected.

@github-actions

github-actions Bot commented Feb 24, 2021

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-11695

@lidavidm

lidavidm commented Feb 24, 2021

Copy link
Copy Markdown
Member Author

Tested with these gRPC versions from conda-forge:

1.36.0
1.35.0
1.34.1
1.33.2
1.30.2

@lidavidm lidavidm force-pushed the arrow-11695-detection branch from faec8f2 to 71b54bc Compare February 25, 2021 00:03
@xhochy

xhochy commented Feb 25, 2021

Copy link
Copy Markdown
Member

It looks like https://github.com/apache/arrow/pull/9569/checks?check_run_id=1974857560#step:5:524 is a valid failure but the integration test seems like a flaky test.

@pitrou

pitrou commented Feb 25, 2021

Copy link
Copy Markdown
Member

Opened grpc/grpc#25556 upstream.

@xhochy

xhochy commented Feb 25, 2021

Copy link
Copy Markdown
Member

Added the patch to the conda recipe and seems to be working fine: conda-forge/arrow-cpp-feedstock#362

@lidavidm lidavidm force-pushed the arrow-11695-detection branch from 71b54bc to 1124853 Compare February 25, 2021 13:18
@lidavidm

lidavidm commented Feb 25, 2021

Copy link
Copy Markdown
Member Author

Pushed a fix for the warning on MacOS.

Thanks Antoine! To be fair to them, we're explicitly using things under a grpc::experimental namespace, but the macros would be nice.

I want to look at the flaky integration test next - that's covered by ARROW-11717.

@pitrou

pitrou commented Feb 25, 2021

Copy link
Copy Markdown
Member

Ah, thanks for pointing it out. I had missed the experimental part.

pitrou
pitrou reviewed Feb 25, 2021
View reviewed changes

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objection from me.

xhochy
xhochy approved these changes Feb 25, 2021
View reviewed changes

@xhochy xhochy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, would merge on green.

@lidavidm

lidavidm commented Feb 25, 2021

Copy link
Copy Markdown
Member Author

Hmm, it seems MacOS is different about this and I'll need to actually include the dummy root cert still. Let me update the PR.

(grpc/grpc#21655 seems related upstream.)

E0225 14:46:36.920509000 4390006208 ssl_utils.cc:568]                  load_file: {"created":"@1614264396.920489000","description":"Failed to load file","file":"/tmp/grpc-20210224-87223-14xfr0o/src/core/lib/iomgr/load_file.cc","file_line":72,"filename":"/usr/share/grpc/roots.pem","referenced_errors":[{"created":"@1614264396.920477000","description":"No such file or directory","errno":2,"file":"/tmp/grpc-20210224-87223-14xfr0o/src/core/lib/iomgr/load_file.cc","file_line":45,"os_error":"No such file or directory","syscall":"fopen"}]}
E0225 14:46:36.920536000 4390006208 ssl_utils.cc:404]                  Could not get default pem root certs.
E0225 14:46:36.920548000 4390006208 tls_security_connector.cc:351]     Update handshaker factory failed.
E0225 14:46:36.920598000 4390006208 ssl_utils.cc:404]                  Could not get default pem root certs.
E0225 14:46:36.920612000 4390006208 tls_security_connector.cc:351]     Update handshaker factory failed.
E0225 14:46:36.920987000 4390006208 tls_security_connector.cc:196]     Client BlockOnInitialCredentialHandshaker not supported yet.
E0225 14:46:36.921110000 123145519153152 ssl_transport_security.cc:1455] Handshake failed with fatal error SSL_ERROR_SSL: error:1408F10B:SSL routines:ssl3_get_record:wrong version number.
E0225 14:46:36.921588000 4390006208 tls_security_connector.cc:196]     Client BlockOnInitialCredentialHandshaker not supported yet.
E0225 14:46:36.921691000 123145519153152 ssl_transport_security.cc:1455] Handshake failed with fatal error SSL_ERROR_SSL: error:1408F10B:SSL routines:ssl3_get_record:wrong version number.
/Users/runner/work/arrow/arrow/cpp/src/arrow/flight/flight_test.cc:2171: Failure
Failed
'client->DoAction(options, action, &results)' failed with IOError: gRPC returned unavailable error, with message: failed to connect to all addresses. Detail: Unavailable

@lidavidm lidavidm force-pushed the arrow-11695-detection branch from 1124853 to e768c3c Compare February 25, 2021 15:09
@lidavidm

lidavidm commented Feb 25, 2021

Copy link
Copy Markdown
Member Author

And now it passes (minus the integration test which I'll look at next).

@xhochy

xhochy commented Feb 25, 2021

Copy link
Copy Markdown
Member

And now it passes (minus the integration test which I'll look at next).

As said before: It seems unrelated to me as I saw that failures in an other PR, too.

@lidavidm

lidavidm commented Feb 25, 2021

Copy link
Copy Markdown
Member Author

And now it passes (minus the integration test which I'll look at next).

As said before: It seems unrelated to me as I saw that failures in an other PR, too.

Sorry, yes, I mean I want to look at it as my next PR (not in this PR). Apologies for the confusion.

@xhochy xhochy closed this in d7da16e Feb 25, 2021
@ravigumm ravigumm mentioned this pull request Jul 24, 2021
Merged
3 tasks
@asfimport asfimport mentioned this pull request Mar 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitrou pitrou pitrou left review comments
@xhochy xhochy xhochy approved these changes

Assignees

No one assigned

Labels

Component: C++ Component: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lidavidm @xhochy @pitrou